Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for nested attributes in groupBy filter #1276

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

ogonkov
Copy link
Contributor

@ogonkov ogonkov commented Mar 29, 2020

Summary

Proposed change:

Add support for nested attributes access for groupBy filter to match Jinja2 behaviour

Closes #1198 .

Checklist

I've completed the checklist below to ensure I didn't forget anything. This makes reviewing this PR as easy as possible for the maintainers. And it gets this change released as soon as possible.

@codecov-io
Copy link

codecov-io commented Mar 29, 2020

Codecov Report

Merging #1276 into master will decrease coverage by 0.11%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
- Coverage   89.53%   89.41%   -0.12%     
==========================================
  Files          22       22              
  Lines        3020     3043      +23     
==========================================
+ Hits         2704     2721      +17     
- Misses        316      322       +6     
Impacted Files Coverage Δ
nunjucks/src/lib.js 83.61% <75.00%> (-1.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8eae5...a5cca7b. Read the comment docs.

@ogonkov ogonkov marked this pull request as ready for review March 29, 2020 08:24
@ogonkov
Copy link
Contributor Author

ogonkov commented Mar 29, 2020

@fdintino should i make PR against your dev branch aswell?

@anaurelian
Copy link

This is the code I'm using to achieve the same effect:

This is a filter I'm using for Nunjucks in Eleventy:

groupByEx: function (arr, key) {
    const result = {};
    arr.forEach(item => {
        const keys = key.split('.');
        const value = keys.reduce((object, key) => object[key], item);

        (result[value] || (result[value] = [])).push(item);
    });
    return result;
}

Sample (actual) usage:

for categoryName, articles in productArticles | groupByEx("data.categoryName")

Acknowledgement: Code based on whatterz/includes.js

@ogonkov
Copy link
Contributor Author

ogonkov commented Apr 7, 2020

Now it should work out of the box (when it gets merged)

@fdintino
Copy link
Collaborator

Don't worry about the conflicts, I'll take care of those. I'll try to set aside some time, if not today, then this week to give feedback or merge.

@fdintino fdintino self-assigned this Apr 21, 2020
@fdintino fdintino self-requested a review April 21, 2020 13:10
@ogonkov
Copy link
Contributor Author

ogonkov commented Apr 21, 2020

@fdintino can you start from reject PR, please? )

@fdintino
Copy link
Collaborator

Sure, will do.

@ogonkov ogonkov force-pushed the groupby_add_nested_access branch 2 times, most recently from c4079d1 to ec6ef6d Compare July 10, 2020 06:45
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2020

Codecov Report

Merging #1276 into master will increase coverage by 0.08%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
+ Coverage   89.56%   89.64%   +0.08%     
==========================================
  Files          22       22              
  Lines        3027     3043      +16     
==========================================
+ Hits         2711     2728      +17     
+ Misses        316      315       -1     
Impacted Files Coverage Δ
nunjucks/src/lib.js 85.88% <94.11%> (+0.81%) ⬆️
nunjucks/src/filters.js 96.20% <100.00%> (ø)
nunjucks/src/compiler.js 95.86% <0.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7087fa9...1e29863. Read the comment docs.

@ogonkov
Copy link
Contributor Author

ogonkov commented Jul 10, 2020

@fdintino this PR is ready for review ;)

nunjucks/src/lib.js Outdated Show resolved Hide resolved
@fdintino
Copy link
Collaborator

A small observation: the behavior here doesn't respect opts.throwOnUndefined. The previous implementation didn't either, so I don't think you need to address that in this PR. It does mean that it behaves differently from jinja2, where attempts to groupby a non-existent attribute raises an exception. It's regrettable that nunjucks doesn't have its own Undefined object, distinct from 'undefined', the way that jinja2 does.

If you did want to use the value of Environment.opts.throwOnDefined, you would be able to get it from this.env.throwOnUndefined in the filter function.

@ogonkov
Copy link
Contributor Author

ogonkov commented Jul 11, 2020

@fdintino i have add support for this option. I just throw it inside function, hope it would be handled somewhere above.

@ogonkov ogonkov requested a review from fdintino July 11, 2020 07:26
@fdintino fdintino force-pushed the groupby_add_nested_access branch from ce7ed46 to 1e29863 Compare July 12, 2020 17:02
@ogonkov
Copy link
Contributor Author

ogonkov commented Jul 12, 2020

@fdintino everything is OK?

@fdintino fdintino merged commit 1e29863 into mozilla:master Jul 13, 2020
@fdintino
Copy link
Collaborator

Yep, everything is fine. Sorry, had to step away for a bit when I was in the middle of preparing for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

groupby complex attributes
5 participants